Fix inconsistent marker boundary order by arkflpc · Pull Request #19995 · ckeditor/ckeditor5

arkflpc · 2026-03-23T15:32:24Z

🚀 Summary

Fixes incorrect order of adjacent marker boundary elements in editing downcast.

📌 Related issues

Closes Incorrect order of adjacent marker boundary elements in editing downcast #19975

💡 Additional information

Optional: Notes on decisions, edge cases, or anything helpful for reviewers.

🧾 Checklists

Use the following checklists to ensure important areas were not overlooked.
This does not apply to feature-branch merges.
If an item is not relevant to this type of change, simply leave it unchecked.

Author checklist

Reviewer checklist

PR description explains the changes and the chosen approach (especially when performance, API, or UX is affected).
The changelog entry is clear, user‑ or integrator-facing, and it describes any breaking changes.
All new external dependencies have been approved and mentioned in LICENSE.md (if any).
All human-readable, translateable strings in this PR been introduced using t() (if any).
I manually verified the change (e.g., in manual tests or documentation).
The target branch is correct.

cursor

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Bugbot Autofix prepared a fix for the issue found in the latest run.

✅ Fixed: Accidentally committed review comment in source file
- Removed the stray internal // REVIEW: note from comparemarkers.ts without altering any functional code.

Or push these changes by commenting:

@cursor push 09d25c103e

Preview (09d25c103e)

diff --git a/packages/ckeditor5-engine/src/conversion/comparemarkers.ts b/packages/ckeditor5-engine/src/conversion/comparemarkers.ts
--- a/packages/ckeditor5-engine/src/conversion/comparemarkers.ts
+++ b/packages/ckeditor5-engine/src/conversion/comparemarkers.ts
@@ -3,7 +3,6 @@
  * For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-licensing-options
  */
 
-// REVIEW: Let's call this file `comparemarkers.ts` (adjust module name and imports)
 /**
  * @module engine/conversion/comparemarkers
  */

_{This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.}

packages/ckeditor5-engine/src/conversion/comparemarkers.ts

…p. Added more tests.

niegowski · 2026-03-30T19:39:57Z

packages/ckeditor5-engine/src/controller/datacontroller.ts

-	// reverse DOM order, and intersecting ranges are in something approximating
-	// reverse DOM order (since reverse DOM order doesn't have a precise meaning
-	// when working with intersecting ranges).
-	result.sort( compareMarkersForDowncast );


This was not needed as the markers map was already sorted in DowncastDispatcher#convert().

scofalik · 2026-03-31T08:54:51Z

packages/ckeditor5-engine/src/conversion/downcastdispatcher.ts

+		// Sort the markers in a stable fashion to ensure that the order in which they are
+		// added to the model's marker collection does not affect how they are
+		// downcast. One particular use case that we are targeting here, is one where
+		// two markers are adjacent but not overlapping, such as an insertion/deletion
+		// suggestion pair representing the replacement of a range of text. In this
+		// case, putting the markers in DOM order causes the first marker's end to be
+		// serialized right after the second marker's start, while putting the markers
+		// in reverse DOM order causes it to be right before the second marker's
+		// start. So, we sort these in a way that ensures non-intersecting ranges are in
+		// reverse DOM order, and intersecting ranges are in something approximating
+		// reverse DOM order (since reverse DOM order doesn't have a precise meaning
+		// when working with intersecting ranges).


I propose to add an example, and maybe shorten the text then. It will be easier to imagine what is going on here.

scofalik · 2026-03-31T08:55:09Z

packages/ckeditor5-engine/src/conversion/downcasthelpers.ts

+		viewWriter.setCustomProperty( 'markerBoundaryType', 'opening', viewStartElement );
+		viewWriter.setCustomProperty( 'markerBoundaryType', 'closing', viewEndElement );


I propose 'start' and 'end' as values, seems simpler, and we always use these two nouns.

…nd" to better reflect used terms.

charlttsie · 2026-04-08T14:40:39Z

I've tested the setData(getData()) and paste operations for performance regressions using editor content with multiple (692) markers, mostly (665) those converted with markerToElement (insertion/removal suggestion markers). The results look good to me - they don't indicate any significant timing differences:

`setData(getData())`

Chrome

Version	Attempt 1	Attempt 2	Attempt 3	Attempt 4	Average (ms)	Change
nightly	1182.7	1191.6	1212.0	1265.2	1212.88
PR	1186.5	1187.9	1189.7	1214.5	1194.65	-1.50%

Firefox

Version	Attempt 1	Attempt 2	Attempt 3	Attempt 4	Average (ms)	Change
nightly	1620.7	1641.6	1701.6	1743.3	1676.82
PR	1636.1	1657.3	1660.5	1763.6	1679.37	+0.15%

Safari

Version	Attempt 1	Attempt 2	Attempt 3	Attempt 4	Average (ms)	Change
nightly	759.0	767.0	806.0	816.0	787.00
PR	757.0	770.0	776.0	817.0	780.00	-0.89%

Paste

Chrome

Version	Attempt 1	Attempt 2	Attempt 3	Attempt 4	Average (ms)	Change
nightly	284.9	285.6	285.7	287.0	285.80
PR	279.0	280.5	281.5	281.6	280.65	-1.80%

Firefox

Version	Attempt 1	Attempt 2	Attempt 3	Attempt 4	Average (ms)	Change
nightly	452.5	461.1	495.5	497.8	476.73
PR	440.1	442.9	452.0	518.8	463.43	-2.79%

Safari

Version	Attempt 1	Attempt 2	Attempt 3	Attempt 4	Average (ms)	Change
nightly	312.0	325.0	337.0	348.0	330.50
PR	329.0	333.0	334.0	342.0	334.50	+1.21%

niegowski · 2026-04-08T15:54:35Z

those converted with markerToElement (insertion/removal suggestion markers).

I'm not sure if you are mentioning the TC insertions/deletions or some custom plugins? AFAIR, TC uses markerToData, not markerToElement. Are we sure that markerToElement is tested?

charlttsie · 2026-04-09T12:44:24Z

You're right, in the scenarios above markerToData was tested, so I retested with markerToElement. I defined custom markers in a way specified in https://ckeditor.com/docs/ckeditor5/latest/api/module_engine_conversion_downcasthelpers-DowncastHelpers.html#function-markerToElement, using the following code snippets:

editor.conversion.for( 'dataDowncast' ).markerToElement( {
     model: 'marker-to-element',
     view: ( markerData, conversionApi ) => {
         const { writer } = conversionApi;

         return writer.createUIElement( 'marker-to-element', {
             name: markerData.markerName.substr( 'marker-to-element:'.length )
         } );
     }
} );

editor.conversion.for( 'editingDowncast' ).markerToElement( {
   model: 'marker-to-element',
   view: ( markerData, conversionApi ) => {
       const { writer } = conversionApi;

       return writer.createUIElement( 'marker-to-element', {
           name: markerData.markerName.substr( 'marker-to-element:'.length )
       } );
   }
} );

and used them in this HTML data, where each <suggestion-start>/<suggestion-end> marker was replaced with that custom marker, so there are 661 such markers in total. For the paste test, I tested pasting the sample content without markers into the document with markers.

Here are the results:

`setData(getData())`

Chrome

Version	Attempt 1	Attempt 2	Attempt 3	Attempt 4	Average (ms)	Change
nightly	896.7	897.4	904.4	904.6	900.78
PR	897.5	904.4	904.7	910.3	904.22	+0.38%

Firefox

Version	Attempt 1	Attempt 2	Attempt 3	Attempt 4	Average (ms)	Change
nightly	1239.2	1239.8	1286.5	1295.6	1265.27
PR	1224.5	1262.2	1273.3	1274.6	1258.66	-0.52%

Safari

Version	Attempt 1	Attempt 2	Attempt 3	Attempt 4	Average (ms)	Change
nightly	584.0	593.0	596.0	597.0	592.50
PR	592.0	594.0	605.0	608.0	599.75	+1.22%

Paste

Chrome

Version	Attempt 1	Attempt 2	Attempt 3	Attempt 4	Average (ms)	Change
nightly	198.2	200.5	200.8	202.4	200.48
PR	199.9	202.1	202.6	206.2	202.70	+1.11%

Firefox

Version	Attempt 1	Attempt 2	Attempt 3	Attempt 4	Average (ms)	Change
nightly	365.3	366.2	379.4	382.0	373.20
PR	373.7	374.8	381.2	394.4	381.02	+2.10%

Safari

Version	Attempt 1	Attempt 2	Attempt 3	Attempt 4	Average (ms)	Change
nightly	243.0	251.0	254.0	257.0	251.25
PR	247.0	252.0	263.0	266.0	257.00	+2.29%

Fix inconsistent marker boundary order

3a1b20b

arkflpc requested review from Mati365 and niegowski March 23, 2026 15:32

Changelog message.

cf2a7d5

cursor bot reviewed Mar 23, 2026

View reviewed changes

packages/ckeditor5-engine/src/conversion/comparemarkers.ts Outdated Show resolved Hide resolved

arkflpc and others added 3 commits March 26, 2026 12:31

Removed leftover comment.

a643568

Adjusted list tests.

5829a2a

Removed double markers sorting in data pipeline downcast. Code cleanu…

18d11c3

…p. Added more tests.

niegowski reviewed Mar 30, 2026

View reviewed changes

scofalik reviewed Mar 31, 2026

View reviewed changes

niegowski added 2 commits March 31, 2026 10:59

Replaced "opening", "closing" custom property values with "start", "e…

f8adbee

…nd" to better reflect used terms.

Updated code comment.

522621c

scofalik approved these changes Mar 31, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix inconsistent marker boundary order#19995

Fix inconsistent marker boundary order#19995
arkflpc wants to merge 7 commits intomasterfrom
ck/19975-marker-boundary-order

arkflpc commented Mar 23, 2026 •

edited

Loading

Uh oh!

cursor bot left a comment •

edited

Loading

Uh oh!

Uh oh!

niegowski Mar 30, 2026

Uh oh!

scofalik Mar 31, 2026

Uh oh!

scofalik Mar 31, 2026

Uh oh!

charlttsie commented Apr 8, 2026

Uh oh!

niegowski commented Apr 8, 2026

Uh oh!

charlttsie commented Apr 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

		viewWriter.setCustomProperty( 'markerBoundaryType', 'opening', viewStartElement );
		viewWriter.setCustomProperty( 'markerBoundaryType', 'closing', viewEndElement );

Uh oh!

Conversation

arkflpc commented Mar 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🚀 Summary

📌 Related issues

💡 Additional information

🧾 Checklists

Author checklist

Reviewer checklist

Uh oh!

cursor bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

niegowski Mar 30, 2026

Choose a reason for hiding this comment

Uh oh!

scofalik Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

scofalik Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

charlttsie commented Apr 8, 2026

setData(getData())

Chrome

Firefox

Safari

Paste

Chrome

Firefox

Safari

Uh oh!

niegowski commented Apr 8, 2026

Uh oh!

charlttsie commented Apr 9, 2026

setData(getData())

Chrome

Firefox

Safari

Paste

Chrome

Firefox

Safari

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

arkflpc commented Mar 23, 2026 •

edited

Loading

cursor bot left a comment •

edited

Loading

`setData(getData())`

`setData(getData())`